Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return to using default closure optimization level #143

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

dibenede
Copy link
Contributor

ADVANCED_OPTIMIZATION mode is breaking CommonJS users. As a spot fix, we'll return to using SIMPLE_OPTIMIZATION mode.

Verified the change by creating a new npm project that depended on the fixed version and deliberately triggered unknown field behavior (skipField) and used uint64s. Couldn't test by enabling CommonJS to run the full binary/*_test.js battery because there are other issues, likely related to our rewriting script and exported test deps.

Should fix #141

ADVANCED_OPTIMIZATION mode is breaking CommonJS users. As a spot fix,
we'll return to using SIMPLE_OPTIMIZATION mode.
@dibenede dibenede self-assigned this Oct 10, 2022
gulpfile.js Show resolved Hide resolved
@dibenede dibenede merged commit 64c19f8 into protocolbuffers:main Oct 10, 2022
@dibenede dibenede deleted the disable-optimizations branch October 10, 2022 19:36
@sampajano
Copy link

sampajano commented Oct 12, 2022

@dibenede @lukesandberg Hi there!

There was a SIGNIFICANT (~5x) reduction in bundle size in 3.21.1 which is unfortunately reverted in 3.21.2.. :)

Screen Shot 2022-10-11 at 5 06 10 PM
(source: https://bundlephobia.com/package/google-protobuf)

I believe the above code size reduction will significantly reduce the overall code size of gRPC-Web users. I wonder if there is a way forward with re-applying the optimization flags? Or do you believe if we have any fundamental blockers here?

Thanks!!

@dibenede
Copy link
Contributor Author

I'm actively working on being able to re-enable the optimizations. The main issue was it seemed to cause serious problems for our CommonJS users in particular (#141).

For now, the problem seems to be getting more stuff tagged for export and fixing up some of the mechanics of how our project generates CommonJS code from handwritten Closure.

So, please bear with us for now and I'll let you know when we're preparing a new release.

@sampajano
Copy link

@dibenede Thanks so much for the update! Very glad that this is being worked on actively! Very much looking forward to it! 😃

@tkaessmann
Copy link

@dibenede Do you have any updates on re-enabling the optimizations for 3.21.2?

@dibenede
Copy link
Contributor Author

dibenede commented Feb 6, 2023

The release is blocked on adding tests to make sure everything is still working. As is, the risk of re-breaking commonjs users is too high.

For users that are willing to experiment (and deal with source), the optimizations can be turned on at HEAD.

@visortelle
Copy link

visortelle commented Feb 22, 2023

@dibenede why not make two builds with different optimization levels and release it?

I often see js modules where import looks like import a from module-a/esm and import a from module-a/cjs.

First random article on this topic: https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

In order to not introduce breaking changes (new import path) for commonjs users, it should be possible to ship two dists, at / and /esm dir.

And add info to the docs like:

In order to reduce bundle size, use the following import path: ...

Correct me if I'm wrong.

@dibenede
Copy link
Contributor Author

The fundamental issue is we lack confidence in the process it took to optimize the code (i.e. manually adding a bunch of @export annotations). So, even if we did a separate release, it could be totally broken for all we know and result in more headaches for everyone.

@visortelle
Copy link

Ah, ok. I misunderstood the root cause of the problem.

@neil-119
Copy link

neil-119 commented Sep 2, 2023

Is there another plan to address the size issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: reader.skipField is not a function
6 participants